Silently skip hook-event when Crow app is not running#234
Merged
dhilgaertner merged 1 commit intomainfrom May 2, 2026
Merged
Conversation
When the Crow app is not running, `crow hook-event` previously exited non-zero with "Socket connection failed: Connection refused", causing Claude Code to surface a noisy "Stop hook error" on every session exit. Hook events are fire-and-forget — a missing listener is an expected state, not an error. Catch SocketError.connectionFailed in the hook-event command only, so other CLI commands still fail loudly when the app is unavailable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dgershman
approved these changes
May 2, 2026
Collaborator
dgershman
left a comment
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None.
Security Review
Strengths:
- Error suppression is scoped strictly to
SocketError.connectionFailedwithinforwardHookEventonly — other CLI commands retain their fail-loud behavior for user-initiated operations. - No new stdin/stdout paths that could leak data; hook payload parsing already existed and the security posture is unchanged.
- The
CROW_SOCKETenv-var override is existing behavior (legacy support) — not introduced here.
Concerns:
- None identified. The fire-and-forget semantics are appropriate for hook events where the app being absent is a normal condition, not a security boundary.
Code Quality
- Clean extraction: the
forwardHookEventfunction has a single responsibility and is independently testable. - The
catch SocketError.connectionFailedpattern correctly matches all associated errno values (bothECONNREFUSEDandENOENT), covering both "app not running" and "socket file absent" scenarios. - Documentation accurately describes the contract (which errors propagate vs. which are swallowed).
- Test uses
setenv/unsetenvwhich modifies process-global state — acceptable here given test speed, but worth noting for future test growth. Adefer { unsetenv(...) }correctly restores state. - All 6 HookEvent tests pass; build is clean with no warnings in the changed files.
Summary Table
| Priority | Issue |
|---|---|
| Green | Consider: if concurrent Swift Testing adoption grows, the setenv-based test could benefit from process isolation, but not blocking today |
Recommendation: Approve — well-scoped fix for #227 with appropriate error handling granularity and good test coverage.
dgershman
approved these changes
May 2, 2026
Collaborator
dgershman
left a comment
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None.
Security Review
Strengths:
- Error handling is scoped narrowly to
SocketError.connectionFailedinsideforwardHookEventonly — other CLI commands continue to fail loudly when the app is unavailable, preserving the correct behavior for user-initiated operations. - No new attack surface: no new inputs, no new IPC methods, no privilege changes.
- The
CROW_SOCKETenv var override in the test is properly cleaned up viadefer { unsetenv(...) }.
Concerns:
- None identified. The change correctly distinguishes between expected (app not running → connection failed) and unexpected (timeouts, write/read failures, JSON-RPC errors) socket conditions.
Code Quality
- Clean extraction: The RPC logic was pulled out of
HookEventCmd.run()into a standaloneforwardHookEvent()function, making it independently testable — good design. - Catch pattern correctness:
catch SocketError.connectionFailedmatches allconnectionFailed(Int32)variants regardless of errno, which is the right behavior — both ECONNREFUSED (app not listening) and ENOENT (socket file absent) should be silently swallowed. - Test coverage:
forwardHookEventSilentWhenAppNotRunningis a clean regression test — points at a non-existent socket viaCROW_SOCKET, verifies the function completes without throwing. - Doc comment on
forwardHookEventclearly explains the rationale and the error taxonomy (which errors propagate vs. which are swallowed).
Summary Table
| Priority | Issue |
|---|---|
| ✅ Green | Consider whether SocketError.createFailed should also be caught — it would fire if the OS can't allocate a socket fd, which is unlikely but also not an app-connectivity issue. Current behavior (letting it propagate) is defensible since it signals a system-level problem. |
Recommendation: Approve — this is a well-scoped, well-tested fix for a real usability issue (#227). The error handling taxonomy is correct and the blast radius is minimal.
This was referenced May 2, 2026
dhilgaertner
added a commit
that referenced
this pull request
May 4, 2026
…xes (#238) Closes #235 ## Summary - Adds `docs/automation.md` as the canonical guide to Settings → Automation and the full auto-flow lifecycle. - Updates `docs/architecture.md` with the dual Ghostty/tmux backend (#229), the new `TerminalRouter` dispatch, the Settings tab split (#228), and the Review Board surface (#188, #205, #207, #210, #212, #220, #226, #231). - Adds `crow rename-terminal` (#206) to `docs/cli-reference.md`. - Adds troubleshooting rows for tmux backend missing, Ghostty surface retry (#218), GitLab nested groups (#233), `GITLAB_HOST` silent skip (#215), auto-respond not firing, and the silent no-op `hook-event` behavior (#234). - Adds `CROW_TMUX_BACKEND` and `CROW_HOOK_DEBUG` to the `docs/configuration.md` env-var table. - Backfills `CHANGELOG.md` `[Unreleased]` with every PR from #137 through #234, grouped by theme. - Updates `README.md` features list and docs index for the new automation suite, review board, terminal renaming, and tmux opt-in. - Appends an "Implementation Status (2026-05)" footer to `docs/terminal-runtime-research.md` noting #229 shipped the headless-PTY backend recommended in the original research. The audit checklist called out as deliverable #1 in the issue is posted as a [comment on #235](#235 (comment)). ## Test plan - [ ] `git diff --stat main` shows only the listed `docs/`, `CHANGELOG.md`, and `README.md` files - [ ] Render each modified doc on GitHub and confirm anchors / cross-links resolve - [ ] Confirm `crow --help` matches the command list in `docs/cli-reference.md` (only `rename-terminal` was missing pre-PR) - [ ] Walk every PR number in the CHANGELOG against `git log --since=2026-04-15 main --oneline` and confirm each one resolves - [ ] Re-export `docs/crow-screenshot.jpeg` against the current Settings/Review-Board UI — **deferred to a follow-up**, called out in the audit comment 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crow hook-eventnow silently exits 0 when the Crow app isn't running, instead of failing with "Socket connection failed: Connection refused".SocketError.connectionFailedinsideHookEventCmdonly — other CLI commands (new-session,set-status, etc.) still fail loudly when the socket is unavailable, since those are user-initiated.Closes #227
Test plan
make buildsucceeds.swift test --package-path Packages/CrowCLI— 35 tests pass, including newforwardHookEventSilentWhenAppNotRunning.echo '{}' | CROW_SOCKET=/tmp/missing.sock .build/debug/crow hook-event --session $(uuidgen) --event Stop→ no output, exit 0.CROW_SOCKET=/tmp/missing.sock .build/debug/crow list-sessionsstill prints "Socket connection failed: …" and exits 1.echo '{...}' | .build/debug/crow hook-event --session <real-uuid> --event Stop→ exit 0, event reaches the app.🤖 Generated with Claude Code